feat: add tokenCacheEnabled to control kernel U2M on-disk token cache - #513
feat: add tokenCacheEnabled to control kernel U2M on-disk token cache#513eric-wang-1990 wants to merge 7 commits into
Conversation
eric-wang-1990
added
the
engineer-bot
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
There was a problem hiding this comment.
Pull request overview
Adds a new tokenCacheEnabled?: boolean connection option for authType: 'databricks-oauth' to control whether the kernel backend’s U2M OAuth flow may use an on-disk refresh-token cache, while keeping “no persistence” as the default posture when migrating to the kernel path.
Changes:
- Plumbs
tokenCacheEnabledthroughbuildKernelConnectionOptionsfor OAuth U2M, defaulting tofalsewhen unset. - Extends the public connection option types/docs to describe the new flag and its kernel/U2M-only behavior.
- Adds unit tests covering default
falseand opt-intruemapping for U2M.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/unit/kernel/auth-u2m.test.ts | Adds assertions/tests to ensure tokenCacheEnabled is forwarded (default false, opt-in true) for kernel OAuth U2M. |
| lib/kernel/KernelAuth.ts | Adds tokenCacheEnabled to the kernel native option type and wires defaulting logic into the U2M mapping. |
| lib/contracts/IDBSQLClient.ts | Exposes tokenCacheEnabled?: boolean on databricks-oauth connection options with API docs. |
| CONNECTION_PARAMETERS.md | Documents the new connection parameter and calls out kernel-only behavior and default. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Token cache is disabled by default for security (silent-no-persist parity); | ||
| // explicitly set to false unless the caller opts in. | ||
| tokenCacheEnabled: oauth.tokenCacheEnabled ?? false, |
There was a problem hiding this comment.
Fixed the misleading error wording and confirmed the mocked U2M auth suite still passes.
Pushed 83b91f0 (bundled with 1 other thread(s)).
| @@ -66,6 +66,7 @@ column. | |||
| | `oauthClientSecret` (M2M) | `string` | ✅ | ✅ | — | M2M client-credentials secret; its **presence** is the U2M-vs-M2M flow selector on both backends (`undefined` ⇒ U2M). Thrift → `DatabricksOAuth.clientSecret`. Kernel → native `oauthClientSecret` (workspace-OIDC M2M) or remapped to `azureClientSecret` (Entra-direct `AzureSpM2m`). A blank/reserved secret is forwarded verbatim and still selects M2M (Thrift parity) — except the Azure SP arm, which rejects it. | | |||
| | `azureTenantId` / `useDatabricksOAuthInAzure` | `string` / `boolean` | ✅ | ⚠️ | — | **Honored on both.** By design the kernel routes **all U2M** (no secret, any cloud) to its cloud-blind in-house OAuth U2M flow — there is no Azure-specific U2M mode, so `useDatabricksOAuthInAzure` is inert on U2M and every Azure workspace (including `.databricks.azure.us` US-gov) is always supported, on any kernel build. `useDatabricksOAuthInAzure` selects only the **M2M** mechanism on an Azure host: absent/`false` → Entra-direct service-principal M2M (native `AzureSpM2m` mode, creds ride `oauthClientId`/`oauthClientSecret`, `azureTenantId` optional — kernel auto-discovers from the workspace `/aad/auth` redirect when omitted); `true` → workspace-OIDC M2M. (`lib/kernel/KernelAuth.ts` `buildKernelConnectionOptions`.) | | |||
| | `persistence` (custom OAuth token store) | `OAuthPersistence` | ✅ | ❌ | — | **Thrift-only.** Kernel throws; it auto-persists U2M tokens to `~/.config/databricks-sql-kernel/oauth/` and does not cache M2M. | | |||
There was a problem hiding this comment.
Resolved with a documentation edit — the reviewer's point was valid and fully actionable in this PR.
Pushed 83b91f0 (bundled with 1 other thread(s)).
eric-wang-1990
added
engineer-bot
Addresses: - #3858665815 at lib/kernel/KernelAuth.ts:840 - #3858665845 at CONNECTION_PARAMETERS.md:68 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Medium
Clean, well-scoped change wiring tokenCacheEnabled through to the kernel U2M path with sensible disable-by-default parity and good unit + interactive-e2e coverage. One medium concern: the regenerated native/kernel/index.js now enforces binding version 0.2.1 while package.json still pins 0.2.0 — an intentional-but-inconsistent split the author already flags as "do not merge" until the 0.2.1 release and pin bump land together.
| const bindingPackageVersion = require('@databricks/databricks-sql-kernel-android-arm64/package.json').version | ||
| if (bindingPackageVersion !== '0.2.0' && process.env.NAPI_RS_ENFORCE_VERSION_CHECK && process.env.NAPI_RS_ENFORCE_VERSION_CHECK !== '0') { | ||
| throw new Error(`Native binding package version mismatch, expected 0.2.0 but got ${bindingPackageVersion}. You can reinstall dependencies to fix this issue.`) | ||
| if (bindingPackageVersion !== '0.2.1' && process.env.NAPI_RS_ENFORCE_VERSION_CHECK && process.env.NAPI_RS_ENFORCE_VERSION_CHECK !== '0') { |
There was a problem hiding this comment.
🟡 Medium — The regenerated napi router now expects binding version 0.2.1 in every platform branch (and the WASI branch at ~L651), but package.json optionalDependencies still pin all 8 kernel packages to 0.2.0. The two were bumped out of lockstep.
The check is gated on NAPI_RS_ENFORCE_VERSION_CHECK, so default npm consumers are unaffected — but any environment that sets that flag will now hit Native binding package version mismatch, expected 0.2.1 but got 0.2.0 and fail to load, because npm installs the pinned 0.2.0. This contradicts the PR's own stated intent to keep consumers on 0.2.0 until 0.2.1 ships.
The PR description acknowledges this ("Do not merge until the 0.2.1 release ships and the pins are bumped"), so this is a state-of-the-PR gate rather than a logic bug — flagging so the version-check bump and the dependency pins land together in the same commit that flips to 0.2.1, not split across the merge boundary.
There was a problem hiding this comment.
⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.
Valid point, but it cannot be actioned in this job. The version-check expects 0.2.1 in native/kernel/index.js (a generated file under the denied native/ path — not writable), while package.json optionalDependencies pin the 8 kernel packages to 0.2.0. Reconciling them requires either reverting the native router to 0.2.0 or bumping the package.json pins to 0.2.1 — but native/ and package.json are both off-limits here, and the reviewer/PR description explicitly gate this on the 0.2.1 kernel release shipping ("Do not merge until 0.2.1 ships and the pins are bumped"). This is a release-coordination / merge-boundary decision for a human: the napi-router version bump and the dependency pins must land together in the commit that flips to 0.2.1, once those packages are published.
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
There was a problem hiding this comment.
Verdict: 1 High
Looks solid — the tokenCacheEnabled wiring in KernelAuth.ts is correctly scoped to the U2M branch, and unit/contract changes are consistent. One high concern: the new e2e test's after cleanup can delete a developer's real kernel OAuth token cache on the skip path, because this.skip() runs before the backup is taken but the after hook still deletes the cache dir unconditionally.
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Addresses: - #3860354328 at tests/e2e/kernel/auth-u2m-e2e.test.ts:111 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
The regenerated native/kernel/index.js baked the expected binding version to 0.2.1, but package.json optionalDependencies still pin all 8 kernel packages to 0.2.0 (no 0.2.1 npm release has shipped). The guard is gated on NAPI_RS_ENFORCE_VERSION_CHECK, so default consumers are unaffected, but any environment that sets the flag would hit "expected 0.2.1 but got 0.2.0" and fail to load. Keep the version literal at 0.2.0 so the loader stays in lockstep with the pinned binding. Bump both the guard literals and the optionalDependencies pins to 0.2.1 together in the follow-up commit once 0.2.1 publishes. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
The connector wires only tokenCacheEnabled through to the kernel U2M path; the passphrase surface is intentionally out of scope for this PR (enable flag only). Remove the tokenCachePassphrase type entry so the committed napi surface matches what the connector actually forwards. It can be re-added alongside real wiring when the passphrase option is surfaced. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
Addresses: - #3860354328 at tests/e2e/kernel/auth-u2m-e2e.test.ts:111 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
b3f1f18 to
f3f9eaf
Compare
GitHub Actions
Queued
GitHub Actions
Queued
GitHub Actions
Queued
GitHub Actions
Queued
There was a problem hiding this comment.
Verdict: 1 Medium
Looks good — clean, correctly U2M-scoped wiring of tokenCacheEnabled (M2M/Azure-SP/PAT paths untouched) with matching unit + e2e coverage. One medium to track: until the 0.2.1 kernel pins land, npm consumers get the 0.2.0 binding that drops the field, so the documented disable-by-default posture silently inverts to cache-enabled on disk (author-flagged as a do-not-merge gate). Nit: the ~/.config/databricks-sql-kernel/oauth/ path in the IDBSQLClient.ts JSDoc and the KernelAuth.ts persistence-rejection message is Linux-only — macOS uses ~/Library/Application Support/... (as the e2e test's own kernelOAuthCacheDir() correctly notes); consider qualifying it as "e.g. (Linux)" to avoid implying it's the universal location.
| @@ -1 +1 @@ | |||
| ef1a6f2dbe72c66a9b6eefc4ca1ff31788f1efa9 | |||
| 628abd6f5045897efcadb38ec77a1e9e0c23544e | |||
There was a problem hiding this comment.
🟡 Medium — The KERNEL_REV bump exposes tokenCacheEnabled in the CI-built binding, but the published optionalDependencies binding stays pinned at @databricks/databricks-sql-kernel@0.2.0, which does not know the field and silently drops it. For npm consumers on the kernel U2M path, dropping the connector-supplied tokenCacheEnabled: false means the kernel falls back to its own default — cache enabled — so the U2M refresh token gets persisted to disk. That is the exact opposite of the "disabled by default / silent-no-persist parity" security posture this PR documents and its unit tests assert. This is a real, non-obvious behavior gap (security-relevant: tokens land on disk unexpectedly), not just a version-string mismatch. The author has already flagged this as a hard "Do not merge until 0.2.1 pins land" gate — surfacing it here so the merge gate isn't lost: confirm the 0.2.1 release has shipped and all 8 platform pins are bumped in the same change before this merges.
What & why
Adds a
tokenCacheEnabled?: booleanconnection option (on thedatabricks-oauthauth type) to control the kernel backend's U2M on-disk OAuth token cache.On the kernel path with OAuth U2M, the Rust kernel can persist the refresh token to an AES-256 encrypted on-disk cache at
dirs::config_dir()/databricks-sql-kernel/oauth/(~/Library/Application Support/...on macOS,~/.config/...on Linux), so a later process skips the interactive browser login. This wires a client option through to the napi binding'stokenCacheEnabledfield.Behavior
tokenCacheEnabled: falseto the kernel — matching Thrift's in-memory (no on-disk persistence) posture, so migrating onto the kernel path doesn't silently start writing tokens to disk. Enabling is opt-in viatokenCacheEnabled: true. (The kernel's own default is enabled; the connector always sendsfalsewhen the option is unset, so disable-by-default is the connector's deliberate choice.)persistence(customOAuthPersistencestore) is untouched and still rejected on the kernel path — this new flag is a separate, simpler control.Dependency
Consumes the napi field
tokenCacheEnabledfrom databricks-sql-kernel #283, now merged to kernelmain(merge commit628abd6). This PR bumpsKERNEL_REV5e5dea9 → 628abd6(additive over #282, so the Azure SP surface is preserved) and regenerates the committed napi binding, which now exposes the field. The kernel-e2e CI path builds the binding fromKERNEL_REV, so it picks up the field.Published npm pins stay at
0.2.0. No@databricks/databricks-sql-kernel0.2.1release has shipped yet, so bumping the 8 platformoptionalDependenciesto a nonexistent version would breaknpm install. Until that release lands (a follow-up commit here bumps the pins), npm consumers still get the0.2.0binding, which silently drops the unknown field — so disable-by-default doesn't take effect for them yet. Do not merge until the0.2.1release ships and the pins are bumped.Testing
Unit
false,true⇒true, and thepersistencerejection still throws. 322 kernel unit tests green against the rebuilt binding.Runtime E2E (new)
Added an interactive U2M E2E suite (
tests/e2e/kernel/auth-u2m-e2e.test.ts), gated behindDATABRICKS_KERNEL_U2M_INTERACTIVE+ workspace host/path so CI still skips it (the browser flow needs a human). It backs up and restores the real on-disk cache dir around the run so a developer's own cached tokens are never destroyed.Ran it against a live Azure pecotesting workspace, completing the browser login for each case:
tokenCacheEnabledunsetSELECT 1= 1tokenCacheEnabled: trueSELECT 1= 1{sha256}.jsonis written ✓Result:
2 passing (17s). Confirms disable-by-default (Thrift parity) and opt-in enable end to end through DBSQLClient → KernelBackend → napi binding → live workspace. (Azure U2M routes through the kernel's cloud-blind in-houseOAuthU2mflow, as expected — it uses the workspace's OIDC-discovered authorize endpoint verbatim.)Related: databricks-sql-kernel #283.
This pull request and its description were written by Isaac.